I think we need a precise definition in the documentation.

What is the difference between timeout and terminate_time?

Is kill_time on top of terminate_time?

What is the semantic difference that leads to one being called timeout and the other just be time?

I want to add a few more comments after having tried to exercise this through in a concrete use case.

It is unclear where the timeouts apply, and thereby it is unclear how they must be used.

Example: I have a process that downloads some information. This information in reported in a json-lines format. The process is batch-mode of some kind (an input triggers a response, continued until stdin closes).

Now there are several layers a user of this helper needs to be aware of.

• the layer that feeds the input/request
• the process that downloads and eventually reports a JSON-line
• the protocol that assembles a JSON-line and decodes it, once complete

Let's assume the remote bandwidth is really low (few bytes per second), and the information that needs to go into a single JSON-line is a few kb, so it needs minutes to assemble a response, and have the "outer" protocol generator yield something.

What would the timeouts apply to? I assume time0 is defined by the input item being fed to the process. Is it now the inner layer (i.e. nothing coming down the remote connection), or the outer layer (no JSON-line-based result yielded for X seconds) that would timeout?

If you use a generator-protocol, then the timeouts that aid the terminating and killing mechanism are only triggered when the context is left. I extended the documentation to clarify that. So for stdout_batchcommand and annexjson_batchcommand, the timeout-counting starts when the context is left, which means when the code in the context decides that all operations are done and exits the context.

Timeout-handling for the transmission should be done in the protocol that is used, e.g. StdOutCaptureGeneratorProtocol. For example, the protocol might send timeout-messages to the result generator and the in-context code can decide what to do. In another example, if the protocol raises an exception in its timeout-callback, the batchcommand-context would be left and the terminating- and killing- timeouts would be started.

I understand the first paragraph and appreciate the clarification.

I have difficulties following the reasoning in the 2nd paragraph. When you say "should be done", you are saying "it is not implemented anywhere"? Do we have any protocol implementation that behaves like this? If not, how do we know that it would work?

I brought it up before, but still think this usage should not be more complex than this. If run() must not have processors= it could be renamed and the replacement could have it.

Thanks. I think I missunderstood your earlier comments.

I have a commit which defines a StdOutCapturePipelineGeneratorProtocol. The protocol takes an optional processor list as a keyword-argument and sends the process-pipeline results to the generator. This is transparent to the run-context-manager and achieves the same terse "ls-files"-implementation, i.e. the code in the context is just:

yield from r

I did not yet put it in because it requires the user of run to select the "right" protocol and to provide a processor list in the protocol_kwargs. That seemed complex to me. And, in contrast to your suggestion (put the pipeline processing into run), which should support any protocol, my approach requires support by the protocol. It would be transparent for the run-context manager code though.

Your suggestion should in principle support any protocol. It would require different treatment of generator- and non-generator protocols, if we want to apply pipelines to non-generator protocols as well. The latter would require the result of the non-generator protocols to adhere to a fixed convention. That seems the be the case, all non-generator protocol results are a dictionary with the keys stdout and stderr. If we want to support processor pipelines on non-generator protocols, the run-context manager would have to pass the values of result_dict['stdout'] and possibly result_dict['stderr'] through the processing pipeline. This would also require a way to specify processor lists for stdout and stderr. Alternatively, we could limit the processors-keyword validity to generator protocols and stdout only.

I am not sure what would be the best approach here. WDYT?

After having worked with adding processing pipelines to non-generator protocols, I would say: "let's not support processing pipelines in non-generator protocols". The non-generator protocols are very much centered around joining individual byte-chunks in their pipe_data_received-method and decoding them into strings in their _prepare_result-method. Without overwriting the _prepare_result-method, it is not even possible to receive undecoded byte-content from a subprocess.

To make use of arbitrary pipelines, pipe_data_received and _prepare_result have to be overridden. That would introduce another "subtree" of protocol-class definitions. Alternatively, we could introduce a "synchronized"-run mode, that would collect all results from a generator-protocol-based run and return them. In fact, such an approach could make the tree of non-generator-protocol definitions obsolete, any generator-protocol could be used in a non-generator way.

I brought the processing protocol classes back. Using those classes the code that executes git ls-files looks like this:

    with run(
            cmd=[
                'git', 'ls-files',
                # we rely on zero-byte splitting below
                '-z',
                # otherwise take whatever is coming in
                *args,
            ],
            protocol_class=StdOutCaptureProcessingGeneratorProtocol,
            stdin=DEVNULL,
            cwd=path,
            protocol_kwargs=dict(
                processors=[
                    decode_processor('utf-8'),
                    splitlines_processor(separator='\0', keep_ends=False)
                ]
            )
    ) as r:
        # This code uses the data processor chain to process data. This fixes
        # a problem with the previous version of the code, where `decode` was
        # used on every data chunk that was sent tp `pipe_data_received`. But
        # data is chunked up randomly and might be split in the middle of a
        # character encoding, leading to weird errors.
        yield from r

None of this documentation is accessible in the renderer docs. This hides the fact that all the data_processors modules are documented in the exact same way, making them indistinguishable from each other.

I think the pattern to manually list the main components in a module in the top-level docstring is not optimal. In the renderer documentation this would e automatic/obvious already. Here it adds content that needs to be kept in sync. I believe an __all__ declaration would be a more appropriate alternative.

In general I think that that sorting of code in all the modules is artificially creating the need to describe what matters most. The "public" component tends to be last in the file (although it could be first).

[...]
In general I think that that sorting of code in all the modules is artificially creating the need to describe what matters most. The "public" component tends to be last in the file (although it could be first).

An old C/C++-habit that is seemingly difficult to shake. But I will manage :-)